Update for model registry operator#125
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughReplaces chart-based Model Registry deployment with an OperatorHub/operator workflow, adds ModelRegistry instance creation steps and parameters, updates namespace/visibility and authorization notes, and adds multiple new Kubeflow how-to docs (pipelines, notebooks, model-registry, tensorboards, volumes-kserve) plus index/intro pages. Kubeflow Trainer v2 minimum Kubernetes version note added. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/en/installation/kubeflow.mdx`:
- Line 217: The docs assume `<model-registry-name>` is known but Kubeflow may
create AuthorizationPolicy resources with different names (e.g.,
`ns-owner-access-istio`), causing "resource not found"; update the instruction
to first list AuthorizationPolicy resources in the target namespace using
`kubectl -n <your-namespace> get authorizationpolicy` to discover the actual
resource name and then run the edit command using that discovered
AuthorizationPolicy resource name (i.e., replace `<model-registry-name>` with
the name returned by the get command).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 717dc992-d273-48f0-8eb6-16ff3a93dc83
📒 Files selected for processing (1)
docs/en/installation/kubeflow.mdx
| **Note: After the Model Registry instance starts, refresh the Model Registry menu in the left navigation of the Kubeflow page to see the instance deployed in the above steps. Before deploying the first instance, the Kubeflow Model Registry interface will display empty.** | ||
|
|
||
| **Note: The Model Registry instance will restrict network requests from non-current namespaces. If you need to allow more namespaces to access, you need to manually modify `kubectl -n <your-namespace> edit authorizationpolicy model-registry-service` and according to the istio documentation, add the namespaces that are allowed to access.** | ||
| **Note: The Model Registry instance will restrict network requests from non-current namespaces. If you need to allow more namespaces to access, you need to manually modify `kubectl -n <your-namespace> edit authorizationpolicy <model-registry-name>` and according to the istio documentation, add the namespaces that are allowed to access.** |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check if the file exists and view the context around line 217
git ls-files docs/en/installation/kubeflow.mdxRepository: alauda/aml-docs
Length of output: 92
🏁 Script executed:
# View the specific line and surrounding context
sed -n '210,225p' docs/en/installation/kubeflow.mdxRepository: alauda/aml-docs
Length of output: 1554
🌐 Web query:
Kubeflow model-registry-operator AuthorizationPolicy naming convention
💡 Result:
In Kubeflow (including when accessing Model Registry through Istio), the Profile controller / KFAM uses these Istio AuthorizationPolicy naming conventions:
- Profile owner policy:
ns-owner-access-istio(kubeflow.org) - Contributor policy (edit/view):
user-<SAFE_USER_EMAIL>-clusterrole-<USER_ROLE>(kubeflow.org)<SAFE_USER_EMAIL>= user email lowercased, with special characters replaced by-(kubeflow.org)<USER_ROLE>=editorview(kubeflow.org)
Citations:
- 1: https://www.kubeflow.org/docs/components/central-dash/profiles/
- 2: https://www.kubeflow.org/docs/components/central-dash/profiles/
- 3: https://www.kubeflow.org/docs/components/central-dash/profiles/
- 4: https://www.kubeflow.org/docs/components/central-dash/profiles/
🌐 Web query:
Kubeflow model-registry ModelRegistry CRD AuthorizationPolicy resource relationship
💡 Result:
-
They’re different layers and don’t “point to” each other.
ModelRegistryis a Kubernetes Custom Resource (CR) (backed by aModelRegistryCRD in operator-style installs) that a controller reconciles into the runtime resources (Deployment/Service/etc.) for a Model Registry instance. (github.com)AuthorizationPolicyis an Istio security resource that controls which authenticated/identified callers can reach workloads (typically at the namespace/workload level). In Kubeflow, Profiles create per-namespace IstioAuthorizationPolicyobjects for the owner and contributors. (kubeflow.org)
-
Where the “relationship” shows up in practice (Kubeflow multi-user):
- Kubeflow recommends installing Model Registry into a user Profile namespace (not the shared
kubeflownamespace) so it fits Kubeflow’s multi-user isolation model. (kubeflow.org) - A Profile namespace automatically gets Istio
AuthorizationPolicyobjects (e.g.,ns-owner-access-istio, and per-contributor policies). These policies commonly apply namespace-wide (no workload selector), so they effectively govern access to all services in that namespace, including the Model Registry API/UI you deployed there. (kubeflow.org)
- Kubeflow recommends installing Model Registry into a user Profile namespace (not the shared
-
So: the
ModelRegistryCR/CRD creates the Model Registry workloads; the Profile-created IstioAuthorizationPolicyrestricts network access to those workloads—without any explicit ownerReference or direct linkage between the two resource kinds. (github.com)
Citations:
- 1: https://github.com/opendatahub-io/model-registry-operator
- 2: https://www.kubeflow.org/docs/components/central-dash/profiles/
- 3: https://www.kubeflow.org/docs/components/model-registry/installation/
- 4: https://www.kubeflow.org/docs/components/central-dash/profiles/
- 5: https://github.com/opendatahub-io/model-registry-operator
Clarify the AuthorizationPolicy identification step to prevent command failures.
The documentation assumes <model-registry-name> is the AuthorizationPolicy resource name, but Kubeflow's Profile controller generates AuthorizationPolicy with different naming conventions (e.g., ns-owner-access-istio). Users following this instruction verbatim will encounter a "resource not found" error. Add a step to identify the correct AuthorizationPolicy before editing:
kubectl -n <your-namespace> get authorizationpolicy
Then use the actual resource name in the edit command.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/en/installation/kubeflow.mdx` at line 217, The docs assume
`<model-registry-name>` is known but Kubeflow may create AuthorizationPolicy
resources with different names (e.g., `ns-owner-access-istio`), causing
"resource not found"; update the instruction to first list AuthorizationPolicy
resources in the target namespace using `kubectl -n <your-namespace> get
authorizationpolicy` to discover the actual resource name and then run the edit
command using that discovered AuthorizationPolicy resource name (i.e., replace
`<model-registry-name>` with the name returned by the get command).
Deploying alauda-ai with
|
| Latest commit: |
1439247
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://566400f0.alauda-ai.pages.dev |
| Branch Preview URL: | https://update-model-registry-instal.alauda-ai.pages.dev |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
docs/en/installation/kubeflow.mdx (1)
217-217:⚠️ Potential issue | 🟡 MinorUse the actual
AuthorizationPolicyname here, not the ModelRegistry name.This command still assumes
<model-registry-name>is the policy resource name, but Kubeflow profile namespaces typically use differentAuthorizationPolicynames. Readers will hitNotFoundunless you first show them how to discover the policy, for example withkubectl -n <your-namespace> get authorizationpolicy, and then edit the returned name.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/en/installation/kubeflow.mdx` at line 217, The docs currently instruct editing `authorizationpolicy <model-registry-name>` but uses the ModelRegistry name which is often incorrect; update the text to tell users to first discover the actual AuthorizationPolicy resource name (e.g., run `kubectl -n <your-namespace> get authorizationpolicy`) and then edit that returned resource (reference the `AuthorizationPolicy` kind and the `kubectl -n <your-namespace> edit authorizationpolicy <policy-name>` command), replacing `<model-registry-name>` with the discovered `<policy-name>` so readers won’t get NotFound errors.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/en/kubeflow/how_to/pipelines.mdx`:
- Around line 43-46: Replace the hardcoded namespace value under
metadata.namespace in the kfp-launcher manifest (currently "wy-testns") with a
clear placeholder like "<your-profile-namespace>" and update the surrounding
text to state explicitly that this value must match the namespace where Pipeline
runs execute so the kfp-launcher ConfigMap is visible to those runs.
In `@docs/en/kubeflow/intro.mdx`:
- Line 10: The note currently instructs setting the namespace PSA to privileged
for Kubeflow broadly; change the wording to avoid recommending a blanket
namespace-wide privileged PSA by scoping the guidance to only the specific
workloads that require lower PSA (e.g., creating Notebook instances) and/or list
the minimal PSA setting per component; update the sentence that mentions
"namespace PSA to privileged" to instead reference targeted workloads (for
example "set PSA to privileged for the Notebook instance namespace or the
specific pod/service account that requires elevated permissions") and add a
brief pointer to the installation guide or a short table describing the minimal
PSA requirement for each Kubeflow component.
---
Duplicate comments:
In `@docs/en/installation/kubeflow.mdx`:
- Line 217: The docs currently instruct editing `authorizationpolicy
<model-registry-name>` but uses the ModelRegistry name which is often incorrect;
update the text to tell users to first discover the actual AuthorizationPolicy
resource name (e.g., run `kubectl -n <your-namespace> get authorizationpolicy`)
and then edit that returned resource (reference the `AuthorizationPolicy` kind
and the `kubectl -n <your-namespace> edit authorizationpolicy <policy-name>`
command), replacing `<model-registry-name>` with the discovered `<policy-name>`
so readers won’t get NotFound errors.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: cadb0e88-1150-4be6-94e6-b5b9dca96930
📒 Files selected for processing (8)
docs/en/installation/kubeflow.mdxdocs/en/kubeflow/how_to/model-registry.mdxdocs/en/kubeflow/how_to/notebooks.mdxdocs/en/kubeflow/how_to/pipelines.mdxdocs/en/kubeflow/how_to/tensorboards.mdxdocs/en/kubeflow/how_to/volumes-kserve.mdxdocs/en/kubeflow/index.mdxdocs/en/kubeflow/intro.mdx
✅ Files skipped from review due to trivial changes (3)
- docs/en/kubeflow/how_to/tensorboards.mdx
- docs/en/kubeflow/how_to/model-registry.mdx
- docs/en/kubeflow/how_to/volumes-kserve.mdx
| metadata: | ||
| name: kfp-launcher | ||
| namespace: wy-testns | ||
| ``` |
There was a problem hiding this comment.
Replace the hardcoded sample namespace before users copy this manifest.
metadata.namespace: wy-testns makes the example non-portable. If readers apply it verbatim, the kfp-launcher ConfigMap lands in the wrong namespace and Pipeline runs in their profile namespace will not see it. Use a placeholder such as <your-profile-namespace> and explicitly say it must match the namespace where the runs execute.
Suggested doc fix
metadata:
name: kfp-launcher
- namespace: wy-testns
+ namespace: <your-profile-namespace>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/en/kubeflow/how_to/pipelines.mdx` around lines 43 - 46, Replace the
hardcoded namespace value under metadata.namespace in the kfp-launcher manifest
(currently "wy-testns") with a clear placeholder like "<your-profile-namespace>"
and update the surrounding text to state explicitly that this value must match
the namespace where Pipeline runs execute so the kfp-launcher ConfigMap is
visible to those runs.
|
|
||
| See [Kubeflow Docs](https://www.kubeflow.org/docs/) for more details about Kubeflow components and features. | ||
|
|
||
| > **NOTE: ** You need to set the namespace PSA to privileged in order to use Kubeflow components. Please contact your cluster administrator to set the namespace PSA to privileged if you encounter permission issues when using Kubeflow components. |
There was a problem hiding this comment.
Avoid documenting privileged PSA as a blanket Kubeflow prerequisite.
This note makes a namespace-wide privileged PSA level sound required for all Kubeflow components, but the installation guide in this PR only scopes PSA lowering to cases like creating Notebook instances. That can steer users toward a broader-than-necessary security posture. Narrow this to the workloads that actually need elevated permissions, or document the minimal PSA setting per component.
🧰 Tools
🪛 LanguageTool
[style] ~10-~10: Consider a more concise word here.
Context: ... to set the namespace PSA to privileged in order to use Kubeflow components. Please contact...
(IN_ORDER_TO_PREMIUM)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/en/kubeflow/intro.mdx` at line 10, The note currently instructs setting
the namespace PSA to privileged for Kubeflow broadly; change the wording to
avoid recommending a blanket namespace-wide privileged PSA by scoping the
guidance to only the specific workloads that require lower PSA (e.g., creating
Notebook instances) and/or list the minimal PSA setting per component; update
the sentence that mentions "namespace PSA to privileged" to instead reference
targeted workloads (for example "set PSA to privileged for the Notebook instance
namespace or the specific pod/service account that requires elevated
permissions") and add a brief pointer to the installation guide or a short table
describing the minimal PSA requirement for each Kubeflow component.
|
/test-pass |
* update for model registry operator * add kubeflow quickstart docs * update * update
2 participants
Summary by CodeRabbit